Skip to content

Allow to keep JWT audience trailing slash #51144

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Nov 23, 2025
Merged

Allow to keep JWT audience trailing slash #51144

merged 1 commit into from
Nov 23, 2025
+107 −5

Conversation

@sberyozkin
Copy link
Member

@sberyozkin sberyozkin commented Nov 20, 2025

Fixes #51133.

I did a mistake 4 years ago in 4a42b44 where the audience property had its trailing slash, if present, removed in OidcCommonUtils#signJwtWithKey, because this is what was necessary to get the Keycloak test passing (Keycloak realm endpoint URL is used as an audience by default), but it is really in application.properties where the users should deal with having the trailing slash present or not.

There is also quarkus.credentials.jwt.audience property that can always be used to customize it.

@sberyozkin sberyozkin changed the title Do not update JWT authentication token audience Do not change configured JWT authentication token audience value Nov 20, 2025
@quarkus-bot

This comment has been minimized.

Sign in to view
michalvavrik
michalvavrik approved these changes Nov 20, 2025
View reviewed changes
Copy link
Member

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to add a property to control the buggy OIDC behavior to an already massive set of existing OIDC props :-)

Personally, I'd call it undocumented behavior. I understand that #51133 has API that accepts aud with the ending slash. And we should give them way to end it with slash other than to add two slashes. I tried to look it up and it seems to be common to have the URI without ending slash.

@quarkus-bot

This comment has been minimized.

Sign in to view
@sberyozkin
Copy link
Member Author

sberyozkin commented Nov 21, 2025

Hi Michal, @michalvavrik

And we should give them way to end it with slash other than to add two slashes.

Right, but users who do not need a slash already have an option not to add it, instead of adding it and expecting it be removed... When it comes to configuring an audience, the configured value, whatever it is, is a user provided value - users have already expressed a requirement with something like http://my.audience/ so asking them to add one more property to retain trailing / seems excessive...

I'm not aware of any OIDC spec text that would require that the JWT audience claim value must not end with /.

Sigh... having said all of that, I think with a property this PR can become bacportable...

Let me think about this property

@sberyozkin sberyozkin force-pushed the jwt_aud_trailing_slash branch from 1649db9 to 6ec9ca6 Compare November 22, 2025 22:38
@quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot

This comment has been minimized.

Sign in to view
@github-actions
Copy link

github-actions bot commented Nov 22, 2025 •
edited
Loading

🙈 The PR is closed and the preview is expired.

michalvavrik
michalvavrik approved these changes Nov 23, 2025
View reviewed changes
Copy link
Member

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@sberyozkin sberyozkin force-pushed the jwt_aud_trailing_slash branch from 6ec9ca6 to 91da575 Compare November 23, 2025 11:46
@sberyozkin
Copy link
Member Author

sberyozkin commented Nov 23, 2025

Thanks @michalvavrik, @gastaldi, so I added a property to allow keeping the trailing slash (default is false as on main) to avoid any release breaking side-effects

@quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot

This comment has been minimized.

Sign in to view
@sberyozkin sberyozkin changed the title Do not change configured JWT authentication token audience value Allow to keep JWT audience trailing slash Nov 23, 2025
@sberyozkin sberyozkin force-pushed the jwt_aud_trailing_slash branch from 91da575 to e04e32b Compare November 23, 2025 18:42
@quarkus-bot
Copy link

quarkus-bot bot commented Nov 23, 2025

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit e04e32b.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

@quarkus-bot
Copy link

quarkus-bot bot commented Nov 23, 2025 •
edited by github-actions bot
Loading

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit e04e32b.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@sberyozkin sberyozkin merged commit 06368d0 into quarkusio:main Nov 23, 2025
33 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.31 - main milestone Nov 23, 2025
@sberyozkin sberyozkin deleted the jwt_aud_trailing_slash branch November 23, 2025 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gastaldi gastaldi gastaldi approved these changes

+1 more reviewer

@michalvavrik michalvavrik michalvavrik approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/documentation area/oidc kind/bugfix triage/backport-3.29

Projects

None yet

Milestone

3.31 - main

Development

Successfully merging this pull request may close these issues.

Audience is modified in jwt when using quarkus.oidc-client.credentials.jwt.audience

3 participants

@sberyozkin @gastaldi @michalvavrik